Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: authentication flow should abort early #888

Merged
merged 8 commits into from
Apr 13, 2024
Merged

Conversation

fmartingr
Copy link
Member

@fmartingr fmartingr commented Apr 13, 2024

  • Ensures that the connection is aborted from the Auth middleware when the authentication is unsuccessful.
  • Modified the main middleware test to ensure that we are not filtering out extra response data
  • Added helper methods to testutil.TestResponse
  • Fixed some typos
  • Fixed tests for the tags and bookmarks api v1

@fmartingr fmartingr self-assigned this Apr 13, 2024
Copy link
Collaborator

@Monirzadeh Monirzadeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi
if you send an unauthorized request it will not reach to the any endpoint in internal/http/routes/api/v1/bookmarks.go
Why is it important?
in #885 for public bookmarks you can't accesses readable content bemuse your request not reach to the endpoint (if be unauthorized)
you can test that in other way
send a update cache request to the

http://127.0.0.1:8080/api/v1/bookmarks/cache

if you are unauthorized than you get 401 but this status code not came from this line

if !ctx.UserIsLogged() {
response.SendError(c, http.StatusForbidden, nil)
return
}

it force us to all api in internal/http/routes/api/v1/bookmarks.go be authorize (and public status be pointless on endpoint in there)
dose you want this?

@fmartingr
Copy link
Member Author

Hi if you send an unauthorized request it will not reach to the any endpoint in internal/http/routes/api/v1/bookmarks.go Why is it important? in #885 for public bookmarks you can't accesses readable content bemuse your request not reach to the endpoint (if be unauthorized) you can test that in other way send a update cache request to the

http://127.0.0.1:8080/api/v1/bookmarks/cache

if you are unauthorized than you get 401 but this status code not came from this line

if !ctx.UserIsLogged() {
response.SendError(c, http.StatusForbidden, nil)
return
}

it force us to all api in internal/http/routes/api/v1/bookmarks.go be authorize (and public status be pointless on endpoint in there) dose you want this?

Right now all handlers under the bookmark endpoint require authentication, so that's good as it is. When we add new mixed handlers we could add the middleware just above the endpoints that require it. Is this something you need for your PR?

@fmartingr
Copy link
Member Author

@Monirzadeh Check dff17e1

@fmartingr
Copy link
Member Author

@Monirzadeh Check dff17e1

Ugh, now I remember why I did it the other way, routes were easier to test.

@Monirzadeh
Copy link
Collaborator

Monirzadeh commented Apr 13, 2024

Hi if you send an unauthorized request it will not reach to the any endpoint in internal/http/routes/api/v1/bookmarks.go Why is it important? in #885 for public bookmarks you can't accesses readable content bemuse your request not reach to the endpoint (if be unauthorized) you can test that in other way send a update cache request to the

http://127.0.0.1:8080/api/v1/bookmarks/cache

if you are unauthorized than you get 401 but this status code not came from this line

if !ctx.UserIsLogged() {
response.SendError(c, http.StatusForbidden, nil)
return
}

it force us to all api in internal/http/routes/api/v1/bookmarks.go be authorize (and public status be pointless on endpoint in there) dose you want this?

Right now all handlers under the bookmark endpoint require authentication, so that's good as it is. When we add new mixed handlers we could add the middleware just above the endpoints that require it. Is this something you need for your PR?

so for example if i need user can acsses readable content of public bookmark (in unauthorized mode) i can't put that in internal/http/routes/api/v1/bookmarks.go
what should i do in this specific situation?

thinking about an app on phone that user can browse public bookmark from multiple shiori server.
so in this situation you need get unauthorized request in endpoint but filter them based on public status in endpoint.

@Monirzadeh
Copy link
Collaborator

Monirzadeh commented Apr 13, 2024

in long term i am thinking about something that user can share there bookmarks with each other (if they want and be disable by default). it is not a mature idea right now.
something like pocket discovery but it work like federation program (mastadon, Pixelfed, etc) based on ActivityPub.

Copy link
Collaborator

@Monirzadeh Monirzadeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is fine to me right now.
we can spend time in that specific situation if it necessary later.
just please fix that failed unit tests.
thanks for this fix 👍

Copy link
Collaborator

@Monirzadeh Monirzadeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so with this new change we should remove this line too

if !ctx.UserIsLogged() {
response.SendError(c, http.StatusForbidden, nil)
return
}

@fmartingr fmartingr changed the title fix: finish connection once we send auth response fix: authentication flow should abort early Apr 13, 2024
Copy link
Collaborator

@Monirzadeh Monirzadeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👍

@fmartingr fmartingr merged commit db313f5 into master Apr 13, 2024
8 checks passed
@fmartingr fmartingr deleted the fix/finish-connection branch April 13, 2024 17:45
@Monirzadeh Monirzadeh added this to the 1.6.3 milestone Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants